fix(cli): preload rich's Unicode width table before in-place upgrade - #1318
Closed
groksrc wants to merge 1 commit into
Closed
fix(cli): preload rich's Unicode width table before in-place upgrade#1318groksrc wants to merge 1 commit into
groksrc wants to merge 1 commit into
Conversation
`bm update` crashed with an unhandled ModuleNotFoundError after Homebrew
replaced the running installation on disk:
ModuleNotFoundError: No module named 'rich._unicode_data.unicode17-0-0'
`_preload_lazy_console_modules` was added for exactly this failure mode,
but it only covered `rich._emoji_codes` and `typer.rich_utils`. rich also
defers its Unicode cell-width table until the first character it cannot
measure with the ASCII fast path in `_cell_len` -- and the status messages
echo captured `brew`/`uv` output, which carries curly quotes and em dashes.
That import lands after `brew upgrade` deleted the prefix we are running
from, so the message meant to report the result became a traceback and a
non-zero exit for an upgrade that had already succeeded.
Resolve the table through `rich.cells.cell_len` rather than importing a
module by name, so rich picks the version and honors UNICODE_VERSION the
same way the print path does.
Also route every update status line through `print_update_status`, which
falls back to a plain write if the console raises. The preload can only
cover the deferred imports known today; once the upgrade has succeeded, a
status line must never be what fails the command.
Fixes #1316
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0112BNQpaC8AujnHc17YzS9D
Member
|
Closing as landed: this fix is on main as 0751139 with your commit and authorship intact — it was bundled into the v0.23.1 fast-follow branch before this PR could be merged directly. It ships in v0.23.1 (releasing today) and is credited to you in the release. Thanks for the excellent diagnosis and fix — the meta_path-based regression test in particular. Sorry for the awkward duplicate-landing path. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #1316.
The bug
bm updatecrashed with an unhandled traceback after Homebrew had already replaced the running installation:_preload_lazy_console_modules()(497a4e0) was written for exactly this failure mode, but it only preloadsrich._emoji_codesandtyper.rich_utils. rich also defers its Unicode cell-width table until the first character it cannot measure with the ASCII fast path in_cell_len— so the import lands afterbrew upgraderemoved the prefix we are executing from, and the message meant to report the result becomes a traceback plus exit code 1 for an upgrade that had already succeeded.Reported on 0.21.5 (which predates the preload entirely), but still reproducible on
main— the added test fails without this change with that exact error.Two details worth keeping in mind while reviewing, both counter-intuitive:
unicode17-0-0is not a Python/Unicode mismatch.rich._unicode_data.load()defaults to"latest"(VERSIONS[-1]), not the interpreter'sunidata_version, so it asks for 17.0.0 even on a py3.14 interpreter reporting Unicode 16.0.0. The file ships in rich; it just was not on disk anymore."Basic Memory is up to date (0.23.0)."never touches the table. TheFAILEDbranch echoing capturedbrewstderr — curly quotes, em dashes,⚠— does.Changes
1. Preload the width table (the actual fix)
Going through
cell_lenrather than importingrich._unicode_data.unicode<version>by name means rich picks the table version itself and honorsUNICODE_VERSIONexactly as the print path does — no hard-coded version to drift, and it works across therich>=13.9.4range inpyproject.toml(older rich has no_unicode_datapackage at all).2. Make the status line unable to fail the command (defense in depth)
New
print_update_status()falls back to a plainprint()if the console raises, and all update status lines incommands/update.pyandmaybe_run_periodic_auto_updatenow go through it.This is the second deferred import found this way, and rich/typer are free to add more. A preload list can only ever cover what we know about today; by the time these lines print, the upgrade has already succeeded, so a status line should never be what turns it into a traceback. Happy to drop this half if you would rather keep the change minimal.
Tests
test_status_message_survives_upgraded_away_install— simulates the deleted prefix with ameta_pathfinder that raises for any not-yet-imported module, then prints a long non-ASCII line (long and non-ASCII on purpose: that is what forces the wrap that reaches for the table). It callsconsole.printdirectly rather thanprint_update_status, so the fallback cannot mask a preload regression._cool_deferred_width_table()— clears rich's caches and unloads the table module first. Without this the test passes whether or not the fix is present, since earlier tests in the session warm the cache. Verified by neutralizing only thecell_lenline: the test then fails with the reportedModuleNotFoundError, and passes with it restored.test_print_update_status_falls_back_to_plain_output— covers the fallback path.tests/cli/test_auto_update.pypasses (33),ruff check/formatclean,ty checkshows no new diagnostics (the 4pymilvusunresolved-import errors are present onmaintoo).Not addressed here
Noted in #1316 as separate observations:
brew upgradeappears to have exited non-zero on an upgrade that landed (frameupdate:25is theFAILEDbranch, yet 0.23.0 installed and the old Cellar directory was removed), which suggests a post-upgrade version re-check before classifying the result would be worthwhile.